-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up files, exports, dependencies #4974
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc. @Andarist for final approval
The |
Linked from webpro-nl/knip#735, thanks for trying Knip! I saw the config and thought I might be able to help out a little. From running Knip in this repo, I was able to fix two bugs, thanks! Re. babel overrides and old husky. Knip automatically picks up Now we can simplify the config a bit: {
"$schema": "https://unpkg.com/knip@5/schema.json",
"workspaces": {
".": {
"entry": ["scripts/*.js"],
"project": ["scripts/*.js"]
}
},
"ignore": [
// used for `#is-development` conditional import
"packages/**/{true,false}.ts",
// file acts as a type test
"packages/xstate-svelte/test/interpreterAsReadable.svelte"
],
"ignoreBinaries": ["svelte-check", "docs:build"],
"ignoreDependencies": [
"@xstate-repo/jest-utils",
"@xstate-repo/jest-utils/setup",
"synckit",
// package.json#exports aren't added as entry points, because `dist/` is .gitignored
"react",
"xstate",
"@types/ws"
]
} There's remaining output:
You could try now using AMA :) |
I assume this might be some issue with pkg.json lookup. Such "package.json scopes" to add |
@webpro Thanks so much for taking a look! Glad you were able to fix two bugs while helping us clean up the config. And also glad to learn about pkg.pr.new, that's quite a useful tool. I was thinking about posting on twitter to ask if I knew any |
I believe this is good to go now 😁 |
Sorry, not sure exactly what you are referring to here. Knip follows the root {
"workspaces": {
"packages/xstate-svelte": {
"typescript": "test/tsconfig.json",
// shorthand for "typescript": { "config": ["test/tsconfig.json"] }
}
}
} |
Since |
Thanks @with-heart! ➕❤️ You could also use the config from my latest comment to get rid of of the ignored |
I guess I just don't understand why this has to be declared as an extra workspace in the first place - couldn't it be picked up automatically? It's fine as a solution right now - I just wonder how Knip's magic sauce is made and if there is any room for improvements on that front |
There's no magic here, Knip just uses |
Right, because it's not a workspace. But including it in Knip's workspaces it feels like we dilute the meaning of a workspace. Multiple |
We're not including it as a workspace, it's only Knip's TypeScript plugin that pulls in
Unfortunately we're simply not there yet. |
Is this one good to go on (pending merge conflicts)? @Andarist @with-heart |
Yeap we're good to go! |
This PR installs and configures
knip
which I then used to clean up a number of things in the repo:export
keyword but aren't imported/re-exportedexport
from vars/types that aren't imported/re-exportedI think it'd be cool to eventually integrate
knip
into our pipeline to be able to automate catching these things but for now doing going through them manually is a good start.P.S.: Since stuff is changing all over the place, it might be easier to read the changes commit-by-commit
P.P.S.: Press n to go to the next commit and press p to go to the previous commit